-
-
Notifications
You must be signed in to change notification settings - Fork 833
Add context menu to TagTile #1743
Add context menu to TagTile #1743
Conversation
With two options: View Community and Remove, which removes the tag from the panel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm other than n^2 worry (and that TagOrderStore is now storing more than just tag ordering)
return () => {}; | ||
} | ||
|
||
removedTags.push(tag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you do all this computation (ie. all the function up until this point) in the async action? That way if you have two in flight at once, you don't lose an update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, we need to calculate removedTags
here for the optimistic update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see - ok, so having traced through how all the async actions and actions creators work again, the contents of the store gets updated as soon as the event dispatch gets around the event loop (ie. once the pending
lands), so the window where you could lose the update is fairly minimal. (Actually I think its the same array object that gets updated each time anyway so possibly it doesn't race at all...)
I guess this is OK then.
src/stores/TagOrderStore.js
Outdated
@@ -165,13 +175,15 @@ class TagOrderStore extends Store { | |||
_mergeGroupsAndTags() { | |||
const groupIds = this._state.joinedGroupIds || []; | |||
const tags = this._state.orderedTagsAccountData || []; | |||
const removedTags = this._state.removedTagsAccountData || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like we may want to turn this into a set (or even store it on the store as a set) to make the membership testing less O(n^2).
Never mind, apparently it's some weird combination of using the unmerged branch of the other PR in combination with the merged branch of this one. Can haz one project? |
With two options: View Community and Remove, which
removes the tag from the panel.